Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenApi : nested ref in #/components/schemas/ #472

Merged
merged 5 commits into from
Sep 26, 2021

Conversation

briceruzand
Copy link
Contributor

Checklist

  • run npm run test and npm run benchmark
  • tests are included

Fix #462

@briceruzand briceruzand changed the title Openapi : nested ref in #/components/schemas/ OpenApi : nested ref in #/components/schemas/ Sep 20, 2021
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

But I would wait for @Eomm comment before merging. He has better understanding on how the $ref works on OpenAPI standard and how to resolve it correctly.

@climba03003 climba03003 requested a review from Eomm September 21, 2021 16:44
@@ -319,9 +319,22 @@ function prepareOpenapiMethod (schema, ref, openapiObject) {
return openapiMethod
}

function prepareOpenapiSchemas (schemas, ref) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you describe what you need to archive here?
I'm not sure to fully understand

With some testing this is not working if I put additional schemas in the bottom test:

    instance.addSchema({ $id: 'OrderItem', type: 'object', properties: { id: { type: 'integer' } } })
+    instance.addSchema({ $id: 'ProductItem', type: 'object', properties: { id: { type: 'integer' } } })
    instance.addSchema({ $id: 'Order', type: 'object', properties: { products: { type: 'array', items: { $ref: 'OrderItem' } } } })
    instance.post('/', { schema: { body: { $ref: 'Order' }, response: { 200: { $ref: 'Order' } } } }, () => {})
+    instance.post('/other', { schema: { body: { $ref: 'ProductItem' } } }, () => {})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Ecomm, I d'like to replace local $ref with local openapi components schema (prefixed by #/components/schemas/) in generated openapi components schema.
With that change, openapi components will be able to reference other local components.

I wrote a simple test of my use case (too simple may be) and I pick the function from cjsewell@1a816ee which seems to work well on my test.

I going to improve my test (with a more complex case, has you done). In try too investigate why the test hang when you add other schema definitions.
Do you have a better idea on that function implementation?

Thx

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have investigated on hanging test you where face to.
I seems to be cause by unfinished fastify-swagger initialization whereas fastify fastify.ready() is ok.
I have rewrote test using async/await to provide more readable tests and I never have this trouble anymore.

Hanging test behavior also exists on your test case without pull request feature.

Copy link
Member

@zekth zekth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like @Eomm mentionned we need to add more test with complexe routing + schemas to try to detect any collision we might have.

Thinking out loud: Isn't the petstore a good playground for this kind of test? https://petstore3.swagger.io/api/v3/openapi.json

 rewrite test using await/async  to be more readable
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1270352819

  • 8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 1237092110: 0.0%
Covered Lines: 554
Relevant Lines: 554

💛 - Coveralls

@briceruzand
Copy link
Contributor Author

@zekth I provide a more complex test case of nested ref in schema definitions.

test('support nested $ref schema : complex case', async (t) => {
const fastify = Fastify()
fastify.register(fastifySwagger, openapiOption)
fastify.register(async (instance) => {
instance.addSchema({ $id: 'schemaA', type: 'object', properties: { id: { type: 'integer' } } })
instance.addSchema({ $id: 'schemaB', type: 'object', properties: { id: { type: 'string' } } })
instance.addSchema({ $id: 'schemaC', type: 'object', properties: { a: { type: 'array', items: { $ref: 'schemaA' } } } })
instance.addSchema({ $id: 'schemaD', type: 'object', properties: { b: { $ref: 'schemaB' }, c: { $ref: 'schemaC' } } })
instance.post('/url1', { schema: { body: { $ref: 'schemaD' }, response: { 200: { $ref: 'schemaB' } } } }, () => {})
instance.post('/url2', { schema: { body: { $ref: 'schemaC' }, response: { 200: { $ref: 'schemaA' } } } }, () => {})
})
await fastify.ready()
const openapiObject = fastify.swagger()
t.equal(typeof openapiObject, 'object')
const schemas = openapiObject.components.schemas
t.match(Object.keys(schemas), ['schemaA', 'schemaB', 'schemaC', 'schemaD'])
// ref must be prefixed by '#/components/schemas/'
t.equal(schemas.schemaC.properties.a.items.$ref, '#/components/schemas/schemaA')
t.equal(schemas.schemaD.properties.b.$ref, '#/components/schemas/schemaB')
t.equal(schemas.schemaD.properties.c.$ref, '#/components/schemas/schemaC')
await Swagger.validate(openapiObject)
})

Thx for your review.

Best regards

Copy link
Member

@zekth zekth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job on this.

@Eomm Eomm merged commit 7496fde into fastify:master Sep 26, 2021
@Eomm Eomm mentioned this pull request Sep 26, 2021
6 tasks
@briceruzand
Copy link
Contributor Author

Thank you all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow nested $ref to resolve for OpenAPI?
7 participants